Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MRG, ENH: Add method to project onto max power ori #7883

Merged
merged 6 commits into from
Jun 23, 2020

Conversation

larsoner
Copy link
Member

@larsoner larsoner commented Jun 9, 2020

Useful for:

  1. Converting pick_ori='vector' solutions to max-power-ori solutions
  2. Eventually for unit-noise-gain vector beamformers we'll want to do this (probably)

In the beamformer PR, we can also check to see the extent to which pick_ori='max-power' agrees with pick_ori='vector' followed by stc.project_onto().

stc_max, directions = stc.project_onto()
flips = np.sign(np.sum(directions * want_nn, axis=1, keepdims=True))
directions *= flips
assert_allclose(directions, want_nn, atol=1e-6)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agramfort here is your proof-by-COBYLA :)

@britta-wstnr
Copy link
Member

I will have a look later whether this performs like what I wrote yesterday (which is how @sarangnemo implemented it in NutMEG back then) - I guess @larsoner got so excited by the plots I shared that he had to try by himself! 😄

For what it's worth, the code I wrote up yesterday performs close to a scalar beamformer with optimal orientation picking - I have some preliminary code to also allow orientations to vary across time, which might make things more interesting, which I'd be happy to contribute.

@larsoner
Copy link
Member Author

I guess @larsoner got so excited by the plots I shared that he had to try by himself!

Yes sorry I got excited about figuring out how to take care of the complex-valued case... :)

I have some preliminary code to also allow orientations to vary across time, which might make things more interesting, which I'd be happy to contribute.

Eventually we can allow this by allowing project_onto take directions of shape n_vertices, 3, n_times as well. I'm not sure if we want to allow it by default, though. At least for the extreme case of doing SVD per time point, one direction will capture 100% of the variance, and the result will be equivalent to just np.linalg.norm(stc.data, axis=1) (i.e., stc.magnitude()) at that point.

In between these two extremes -- i.e., not doing SVD across all time points, or SVD for each individual time point -- things will vary a bit, but I'd be tempted to have people .crop and project_onto and recombine if they want to try this sort of thing. Before adding it in MNE we'd want to look to see the extent to which it's useful in practice. The SVD-across-all-time approach here already makes sense because it enables things like envelope correlation computation with free-orientation (typically volumetric) minimum norm inverse solutions, which is not really doable currently in master (and we also know we'll need it for some beamformer implementations, as you say).

@britta-wstnr
Copy link
Member

britta-wstnr commented Jun 17, 2020

image

So our methods agree - left image is my version (minus polarity flipping), right image is your version !

(Data is cropped toy data, so not necessarily meaningful output, just to compare numbers.)

@larsoner
Copy link
Member Author

Excellent -- in that case this is good to go @agramfort . Once this is merged I'll rebase the beamformer refactoring PR and we can properly test the two vector "unit-noise-gain" methods with and without SVD afterward

Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quite skeptical about the API.

You never use the stc.project_onto with directions != None in any example or tutorial.

should directions be a public parameter?

we have stc.normal so should we rather of stc.principal_component?

stc : instance of SourceEstimate
The projected source estimate.
normals : ndarray, shape (n_vertices, 3)
Only returned if ``normals is None``, the normals computed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Only returned if ``normals is None``, the normals computed
Only returned if ``directions is None``.

@larsoner
Copy link
Member Author

should directions be a public parameter?

It was easy enough to add and validation was simple. People could want to for example project onto the max power orientations they got from some other filter. So I don't see a problem/maintenance cost from keeping it.

we have stc.normal so should we rather of stc.principal_component?

Starting from scratch I'd actually prefer the API to be stc.project_onto('normal' | 'pca') or so. I can rename it to principal_component but if we think of other ways to do the projections our namespace gets more annoying -- I'd rather keep and potentially in the future add additional options to a generic project_onto function...

@agramfort
Copy link
Member

ok I propose to deprecate VectorSourceEstimate.normal and introduce

VectorSourceEstimate.project(directions, src=None, use_cps=True, return_directions=False)

where directions can be
'normal' -> requires src
'pca' -> does not require src? and use return_directions if you want to get the estimated directions
array of shape (n_dipoles, 3)

sounds like a plan?

@larsoner
Copy link
Member Author

I'd rather just always return the directions, return_ kwargs I'd rather leave as a mechanism for backward compatibility. It's very easy to do [0] to get the first output of a function.

src is mandatory in both cases to make the sign flip directions not arbitrary

@larsoner
Copy link
Member Author

(deal with sign flip ambiguity in pca)

@agramfort
Copy link
Member

agramfort commented Jun 23, 2020 via email

@larsoner
Copy link
Member Author

Yep, I'll push shortly

@agramfort agramfort merged commit 181a572 into mne-tools:master Jun 23, 2020
@agramfort
Copy link
Member

Thx @larsoner

@larsoner larsoner deleted the vec-svd branch June 23, 2020 18:40
larsoner added a commit to larsoner/mne-python that referenced this pull request Jun 23, 2020
* upstream/master:
  MRG, ENH: Add method to project onto max power ori (mne-tools#7883)
  WIP: Warn if untested NIRX device (mne-tools#7905)
  MRG, BUG: Fix bug with volume morph and subject_to!="fsaverage" (mne-tools#7896)
  MRG, MAINT: Clean up use of bool, float, int (mne-tools#7917)
  ENH: Better error message for incompatible Evoked objects (mne-tools#7910)
  try to fix nullcontext (mne-tools#7908)
larsoner added a commit to larsoner/mne-python that referenced this pull request Jun 25, 2020
* upstream/master: (23 commits)
  MAINT: Add mne.surface to docstring tests (mne-tools#7930)
  MRG: Add smoothing controller to TimeViewer for the notebook backend (mne-tools#7928)
  MRG: TimeViewer matplotlib figure color (mne-tools#7925)
  fix typos (mne-tools#7924)
  MRG, ENH: Add method to project onto max power ori (mne-tools#7883)
  WIP: Warn if untested NIRX device (mne-tools#7905)
  MRG, BUG: Fix bug with volume morph and subject_to!="fsaverage" (mne-tools#7896)
  MRG, MAINT: Clean up use of bool, float, int (mne-tools#7917)
  ENH: Better error message for incompatible Evoked objects (mne-tools#7910)
  try to fix nullcontext (mne-tools#7908)
  WIP: Fix Travis (mne-tools#7906)
  WIP: Prototype of notebook viz (screencast) (mne-tools#7758)
  MRG, FIX: Speed up I/O tests, mark some slow (mne-tools#7904)
  Proper attribution for Blender tutorial (mne-tools#7900)
  MAINT: Check usage [ci skip] (mne-tools#7902)
  Allow find_bad_channels_maxwell() to return scores (mne-tools#7845)
  Warn if NIRx directory structure has been modified from original format (mne-tools#7898)
  Pin pvyista to 0.24.3 (mne-tools#7899)
  MRG: Add support for reading and writing sufaces to .obj (mne-tools#7824)
  Fix _auto_topomap_coords docstring. (mne-tools#7895)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants